Skip to content

Resolve a write when it commits, not when the request succeeds (#250) - #253

Merged
adulbrich merged 1 commit into
mainfrom
fix/kv-durable-writes
Aug 5, 2026
Merged

Resolve a write when it commits, not when the request succeeds (#250)#253
adulbrich merged 1 commit into
mainfrom
fix/kv-durable-writes

Conversation

@adulbrich

Copy link
Copy Markdown
Collaborator

Closes #250.

The defect

src/lib/storage/kv.ts's run() resolved on request.onsuccess. In IndexedDB a request succeeding and its transaction committing are separate events, in that order, so a transaction that aborts at commit time -- quota exhaustion being the realistic cause -- fired onabort after the promise had already settled, where reject() is a no-op.

The comment above it claimed to handle exactly the case it did not.

Why it matters

Every write to this database goes through run(): putDocument, putFile, putBlob, deleteDocument, deleteFile, deleteBlob. A quota-aborted save of a preset reported success and lost the data, and because readJson in app-storage.ts swallows read errors into a fallback, the loss surfaced later as a missing preset rather than as an error at save time.

#249 raised the odds of hitting it: it added a RAW conversion cache of up to 2 GB to the same origin, and it writes through putBlob.

The fix

Resolve on transaction.oncomplete, which is the durable signal, capturing request.result in onsuccess since it is only valid inside its own handler. This is what updateDocument already did.

Verification

The RED run is the whole argument. Quota cannot be exhausted in fake-indexeddb, so the tests reproduce the ordering that matters -- the request succeeds, the transaction dies afterwards -- by aborting from a listener on the successful request:

● rejects rather than reporting the write as durable
    expect(received).rejects.toThrow()
    Received promise resolved instead of rejected
    Resolved to value: "aborted-preset"

...while the companion test asserting the data is gone passed. That pairing is the bug in one screen: the write was rolled back, and the promise resolved with the key anyway.

Four tests in kv-durable-writes.test.ts. The two that guard already-correct behaviour were validated by deliberately breaking the production code:

  • the updateDocument guard fails as expected when updateDocument resolves at put time
  • the mid-flight guard still passed with request.onerror removed, so it was renamed to what it actually pins ("rejects rather than hanging") and the finding recorded in a comment rather than left behind a misleading name

Reviewed for, and cleared

Moving resolution to oncomplete means the transaction is dead when the caller sees the value, so any run() action issuing a second request would now get TransactionInactiveError. All ten call sites audited: every action is exactly one request, and run is module-private and never leaks the store or transaction past its synchronous callback. The one genuinely multi-request function, updateDocument, does not use run().

Reads share run() and so settle a tick later too. Harmless -- a readonly transaction commits right after its last request -- and noted in the code rather than left for a future reader to wonder about.

npx jest: 58 suites, 388 tests, all pass. ultracite check and tsc --noEmit clean.

🤖 Generated with Claude Code

…ucceeds

`run()` resolved on `request.onsuccess`. In IndexedDB a request succeeding
and its transaction committing are separate events, in that order, so a
transaction that aborts at commit time -- quota exhaustion being the
realistic cause -- fired `onabort` after the promise had already settled,
where `reject()` is a no-op. The comment above it claimed to handle exactly
the case it did not.

Every write to this database goes through `run()`: `putDocument`, `putFile`,
`putBlob`, `deleteDocument`, `deleteFile`, `deleteBlob`. So a quota-aborted
save of a preset reported success and lost the data, and because `readJson`
in `app-storage.ts` swallows read errors into a fallback, the loss surfaced
later as a missing preset rather than as an error at save time.

Resolving on `transaction.oncomplete` is the durable signal, which is what
`updateDocument` already does. The result now has to be captured in
`onsuccess` and handed over at commit, since `request.result` is only valid
inside its own handler. Reads share `run()`, so they settle a tick later
too; every action it issues is a single request, so no caller is left
holding a transaction that has since committed.

#249 makes this worth fixing now rather than later: it added a RAW
conversion cache of up to 2 GB to the same origin, and it writes through
`putBlob`, so it both raises quota pressure and is subject to the same
false success.

Tests cover the abort paths, which had none.

Closes #250

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lumilab Ready Ready Preview Aug 2, 2026 4:32am

@adulbrich
adulbrich merged commit eafc0f4 into main Aug 5, 2026
12 checks passed
@adulbrich
adulbrich deleted the fix/kv-durable-writes branch August 5, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kv.ts run() reports a write as durable before the transaction commits

1 participant